Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
93bc16e to
1cc00a5
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
|
This pr can now be reviewed im done with my stuff |
| }); | ||
| } | ||
|
|
||
| const existing = sessions.get(input.threadId); |
There was a problem hiding this comment.
🟢 Low Layers/CopilotAdapter.ts:1679
Concurrent startSession calls for the same threadId race at the sessions.get check (line 1679): both callers find no existing session, both create new clients and sessions, and the second sessions.set (line 1778) overwrites the first. The first session's client and session become unreachable and leak. Consider synchronizing in-flight session creation, e.g., by tracking pending promises in a separate map keyed by threadId.
🤖 Copy this AI Prompt to have your agent fix this:
In file apps/server/src/provider/Layers/CopilotAdapter.ts around line 1679:
Concurrent `startSession` calls for the same `threadId` race at the `sessions.get` check (line 1679): both callers find no existing session, both create new clients and sessions, and the second `sessions.set` (line 1778) overwrites the first. The first session's client and session become unreachable and leak. Consider synchronizing in-flight session creation, e.g., by tracking pending promises in a separate map keyed by `threadId`.
Evidence trail:
apps/server/src/provider/Layers/CopilotAdapter.ts line 844: `const sessions = new Map<ThreadId, ActiveCopilotSession>();` - plain Map with no synchronization
apps/server/src/provider/Layers/CopilotAdapter.ts line 1679: `const existing = sessions.get(input.threadId);` - the check
apps/server/src/provider/Layers/CopilotAdapter.ts lines 1720-1726: `yield* validateSessionConfiguration(...)` - first async yield point between check and set
apps/server/src/provider/Layers/CopilotAdapter.ts lines 1728-1763: `yield* Effect.tryPromise(...)` - second async yield point between check and set
apps/server/src/provider/Layers/CopilotAdapter.ts line 1778: `sessions.set(input.threadId, record);` - the set that overwrites any racing calls
| ...(input.cwd ? { cwd: input.cwd } : {}), | ||
| logLevel: "error", | ||
| }; | ||
| const client = options?.clientFactory?.(clientOptions) ?? new CopilotClient(clientOptions); |
There was a problem hiding this comment.
🟡 Medium Layers/CopilotAdapter.ts:1705
If session creation fails after client is instantiated (lines 1720–1778), the CopilotClientHandle is never cleaned up because client.stop() is only called in stopRecord, which requires the session to be in the sessions map. Since CopilotClient spawns a subprocess, this leaks orphan processes when validateSessionConfiguration fails or Effect.tryPromise throws. Consider wrapping client creation in Effect.acquireRelease to ensure client.stop() runs on failure.
- const client = options?.clientFactory?.(clientOptions) ?? new CopilotClient(clientOptions);
+ const client = yield* Effect.acquireRelease(
+ Effect.sync(() => options?.clientFactory?.(clientOptions) ?? new CopilotClient(clientOptions)),
+ (client) => Effect.promise(() => client.stop())
+ );🤖 Copy this AI Prompt to have your agent fix this:
In file apps/server/src/provider/Layers/CopilotAdapter.ts around line 1705:
If session creation fails after `client` is instantiated (lines 1720–1778), the `CopilotClientHandle` is never cleaned up because `client.stop()` is only called in `stopRecord`, which requires the session to be in the `sessions` map. Since `CopilotClient` spawns a subprocess, this leaks orphan processes when `validateSessionConfiguration` fails or `Effect.tryPromise` throws. Consider wrapping client creation in `Effect.acquireRelease` to ensure `client.stop()` runs on failure.
Evidence trail:
apps/server/src/provider/Layers/CopilotAdapter.ts lines 1700-1705 (client instantiation), lines 1720-1725 (validateSessionConfiguration call), lines 1460-1527 (validateSessionConfiguration definition with client.start() at line 1471 and multiple failure points), lines 1727-1759 (session creation that can fail), lines 1764-1772 (record added to sessions only after all succeeds), lines 1639-1661 (stopRecord function definition), line 1647 (only location where client.stop() is called). No Effect.acquireRelease/ensuring/onInterrupt patterns found via git_grep.

What Changed
Why
UI Changes
Checklist
Note
Add GitHub Copilot as a supported provider across server, web, and contracts
CopilotAdapter.tsthat wires@github/copilot-sdkinto the provider system, handling session lifecycle, turn tracking, event streaming, and model switching.model.tsand updates contract schemas inorchestration.tsto accept'copilot'as a valid provider kind.appSettings.ts), the settings UI (_chat.settings.tsx), the composer (composerProviderRegistry.tsx), and the provider/model picker to expose Copilot configuration including CLI path, config directory, and custom models.copilotCliPath.ts) that locates the bundled Copilot binary or npm loader across platforms and packaging layouts (Node/Electron).Macroscope summarized 5c6b51e.
Note
Medium Risk
Introduces a new runtime provider backed by
@github/copilot-sdk, including session/event mapping, model/effort validation, and a startup health probe that spawns/controls a Copilot client, which could impact stability across platforms.Overview
Adds GitHub Copilot as a first-class provider end-to-end: new server
CopilotAdapterlayer (session lifecycle, turn streaming, tool/plan/usage event normalization, approvals + user-input requests, model switching with reasoning-effort validation) and wires it intoProviderAdapterRegistry, server layers, and persisted session provider decoding.Introduces Copilot CLI discovery/override support (
copilotCliPath,copilotConfigDir) with bundled binary resolution, plus a Copilot provider health check that starts the SDK client with a platform-aware timeout.Updates the web app to expose Copilot in provider pickers/settings and to store/normalize Copilot-specific model options and custom models; also tweaks timeline rendering for tool/file-change entries (icon/title fallbacks and preferring
changedFilespreviews) and adds/adjusts tests accordingly.Written by Cursor Bugbot for commit 5c6b51e. This will update automatically on new commits. Configure here.